Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

checkpatch: Remove ENOSYS check #29806

Closed
wants to merge 1 commit into from

Conversation

ceolin
Copy link
Member

@ceolin ceolin commented Nov 5, 2020

Zephyr uses ENOSYS for all not implemented functions and not only for
syscalls. So checkpatch generates invalid warnings about it.

https://github.com/zephyrproject-rtos/zephyr/wiki/Naming-Conventions#return-codes

Signed-off-by: Flavio Ceolin flavio.ceolin@intel.com

@ceolin ceolin requested review from nashif and galak November 5, 2020 00:46
Zephyr uses ENOSYS for all not implemented functions and not only for
syscalls. So checkpatch generates invalid warnings about it.

https://github.com/zephyrproject-rtos/zephyr/wiki/Naming-Conventions#return-codes

Signed-off-by: Flavio Ceolin <flavio.ceolin@intel.com>
@jukkar
Copy link
Member

jukkar commented Nov 5, 2020

Hmm, I thought we return ENOTSUP if the function is not implemented. I could see only two returns using ENOSYS.

$ ack --cc ENOSYS drivers/ subsys/ | wc
      2       6     112
$ ack --cc ENOTSUP drivers/ subsys/ | wc
   1036    3357   57294

@ceolin
Copy link
Member Author

ceolin commented Nov 5, 2020

Hmm, I thought we return ENOTSUP if the function is not implemented. I could see only two returns using ENOSYS.

$ ack --cc ENOSYS drivers/ subsys/ | wc
      2       6     112
$ ack --cc ENOTSUP drivers/ subsys/ | wc
   1036    3357   57294

I thought we were using ENOTSUP as well, but someone pointed it out in my pr during a review. It seems we have an inconsistency between documentation here and what we are doing then.

@pabigot
Copy link
Collaborator

pabigot commented Nov 17, 2020

Zephyr uses ENOSYS for all not implemented functions and not only for syscalls.

This statement is incorrect (and the wiki is way out of date). See #23727 for what's really going on.

If checkpatch isn't actually complaining about something that we care about I don't think we should be changing it just on the basis of outdated documentation.

@ceolin
Copy link
Member Author

ceolin commented Nov 17, 2020

Zephyr uses ENOSYS for all not implemented functions and not only for syscalls.

This statement is incorrect (and the wiki is way out of date). See #23727 for what's really going on.

If checkpatch isn't actually complaining about something that we care about I don't think we should be changing it just on the basis of outdated documentation.

@pabigot I'm really not understanding you. In a different pr you told me that ENOSYS should be a better return for a function that is not a syscall what is aligned with this wiki. Checkpatch complains about it, so that is why I created this pr. Now you are saying that wiki is outdated ....

@ceolin
Copy link
Member Author

ceolin commented Nov 17, 2020

Zephyr uses ENOSYS for all not implemented functions and not only for syscalls.

This statement is incorrect (and the wiki is way out of date). See #23727 for what's really going on.
If checkpatch isn't actually complaining about something that we care about I don't think we should be changing it just on the basis of outdated documentation.

@pabigot I'm really not understanding you. In a different pr you told me that ENOSYS should be a better return for a function that is not a syscall what is aligned with this wiki. Checkpatch complains about it, so that is why I created this pr. Now you are saying that wiki is outdated ....

btw, if you were complaining about the "all not implemented functions" I agree that this statement is wrong, but the change itself seems fine for me

@pabigot
Copy link
Collaborator

pabigot commented Nov 17, 2020

Zephyr uses ENOSYS for all not implemented functions and not only for syscalls.

This statement is incorrect (and the wiki is way out of date). See #23727 for what's really going on.
If checkpatch isn't actually complaining about something that we care about I don't think we should be changing it just on the basis of outdated documentation.

@pabigot I'm really not understanding you. In a different pr you told me that ENOSYS should be a better return for a function that is not a syscall what is aligned with this wiki. Checkpatch complains about it, so that is why I created this pr.

I don't remember exactly where that was but I think what I was trying to say was that -ENOSYS was better in theory, but it had to be done consistently within the power management subsystem, and I was probing the possibility of reaching agreement on doing that. Without agreement, which I think is unlikely for reasons identified in #23727, it's probably not right to use -ENOSYS.

Now you are saying that wiki is outdated ....

Well, yeah. That wiki page was last modified over three years ago. In fact the whole subject of that page is being revisited in #29569.

Whatever we come up with for standards belongs in the doc/ directory where people can find it and it's maintained in concert with the code.

I'm not in favor of removing the check from checkpatch, because it does seem to be kicking up noise when something is done that's not the way that Zephyr actually works today, so it's a good signal. But if it is to be removed, then the commit message would need to be updated, because what it gives as rationale for removing the check is simply not true.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants